New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use custom RNG for sketch path #5728
Conversation
👍 once we can use C++11 I would prefer to use stl random lib but this seems like a suitable solution at the moment |
looks like xkcd tests are failing (as we changed the random number generator). |
Ah, of course. I'm not going to try to make it backward compatible, and just update the images :) Honestly, it's probably a good thing -- the C stdlib RNG doesn't guarantee any consistency across compilers anyway, but this should. |
The xkcd test have always been failing for me on OSX so that seems likely. Would be great if this fixes that too. |
👍 to not worrying about back compatibility here. I really really hope no one is relying on the details of the sketch path! |
appveyor is not failing when it fails, but this is showing up, it used to just fail on pixel errors.
|
ex
from https://ci.appveyor.com/project/mdboom/matplotlib/build/1.0.338/job/nl9a32g98n9o9rov |
double get_double() | ||
{ | ||
m_seed = (a * m_seed + c); | ||
return (double)m_seed / (double)(1L << 32); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is double 32 or 64bits? I wonder if this is what is causing the test failure on windows (in that this is no longer scaled [-1, 1] on all platforms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
double is 64 bits, so it looks to me like this should yield numbers from 0 to (2^32-1)/2^32. I can imagine the result of the division could differ depending on whether it is done using plain double-precision, or using extra-width floating point registers. Use of denormalized numbers is another option. There are compiler switches that can affect floating point results, so the same code on the same hardware can yield different results depending on the compiler and its options. Maybe this is what is happening here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the 1L
needs to be 1LL
to ensure it is 64 bits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be scaled [0, 1) on all platforms, since the seed is an unsigned int. I think @efiring is right here, that it should be 1LL. Let's try that and run it over the Windows tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would certainly be consistent with a Linux vs. Windows problem. On a 64-bit machine, 1L is 64-bits on Linux and 32-bits on Windows. 1LL is 64 bits on both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to spam -- reading further, however, the test failures on Windows are prior to this change, right? That could easily be explained by using the C stdlib RNG which varies entirely from platform to platform. This should (and seemingly does) address that problem by using exactly one RNG implementation everywhere.
Use custom RNG for sketch path
Use custom RNG for sketch path Conflicts: lib/matplotlib/tests/baseline_images/test_path/xkcd.pdf lib/matplotlib/tests/baseline_images/test_path/xkcd.png lib/matplotlib/tests/baseline_images/test_path/xkcd.svg Resolved all by copying binaries from master branch
backported as 3be4490 |
This is an alternative to #5725.